Hi @frittentheke. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

@frittentheke what about moving the provider specific args into the provider variable as has already been done for webhook. For example.

provider:
  name: aws

  aws:
    customArg: good-default
    otherCustomArg:

This would be a better pattern for the common use cases as the inputs could be defined in the schema.

If you do want to update the extraArgs implementation, it probably needs to support a map of strings or string lists as idiomatically some args can be repeated.

extraArgs:
  my-arg: test
  my-other-arg:
    - foo
    - bar

@frittentheke what about moving the provider specific args into the provider variable as has already been done for webhook. For example.

provider:
  name: aws

  aws:
    customArg: good-default
    otherCustomArg:

This would be a better pattern for the common use cases as the inputs could be defined in the schema.

Yes. I am wondering if this would need to happen for all built-in providers in one PR to not make this more complex then necessary (by having e.g. AWS have all its options explicitly defined and others still only via extraArgs?

If you do want to update the extraArgs implementation, it probably needs to support a map of strings or string lists as idiomatically some args can be repeated.

extraArgs:
  my-arg: test
  my-other-arg:
    - foo
    - bar

Wow good catch, thank you @stevehipwell
I'll push this change soon. Maybe extraArgs to also potentially be a map is a good idea no matter if the provider specific configuration is moved to its own values / data structure.

Yes. I am wondering if this would need to happen for all built-in providers in one PR to not make this more complex then necessary (by having e.g. AWS have all its options explicitly defined and others still only via extraArgs?

I don't think so, we already document providers with configuration support in the chart README. But a provider being added should be fully implemented with a typed JSON schema otherwise it doesn't make much sense.

Wow good catch, thank you @stevehipwell
I'll push this change soon. Maybe extraArgs to also potentially be a map is a good idea no matter if the provider specific configuration is moved to its own values / data structure.

Yes I agree that adding map support makes sense, there are likely cases where providers don't have specific configuration or where extra args are needed too.

Any chance to cover this funcitonality with helm unit tests as well?

/ok-to-test

@stevehipwell I pushed a new revision, including a unit test which @ivankatliarchuk asked for.

PTAL

need to re-run linters

/label tide/merge-method-squash

New changes are detected. LGTM label has been removed.

Sorry @ivankatliarchuk I missed to run helm-docs again causing the CI lint stage to fail.
Should (fingers crossed) be good now.

/test pull-external-dns-unit-test

Thanks for the quick responses @frittentheke, the changes look good. Sorry for missing this previously but please could you add an entry to the chart CHANGELOG.

Thanks for the quick responses @frittentheke, the changes look good. Sorry for missing this previously but please could you add an entry to the chart CHANGELOG.

DONE @stevehipwell

Thanks for your patience @frittentheke.

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk, stevehipwell

The full list of commands accepted by this bot can be found here.

The pull request process is described here